-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add the BestFriend
attribute for restricting cross-assembly internal access
#1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -5,3 +5,4 @@ | |||
using System.Runtime.CompilerServices; | |||
|
|||
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.TestFramework, PublicKey=002400000480000094000000060200000024000052534131000400000100010015c01ae1f50e8cc09ba9eac9147cf8fd9fce2cfe9f8dce4f7301c4132ca9fb50ce8cbf1df4dc18dd4d210e4345c744ecb3365ed327efdbc52603faa5e21daa11234c8c4a73e51f03bf192544581ebe107adee3a34928e39d04e524a9ce729d5090bfd7dad9d10c722c0def9ccc08ff0a03790e48bcd1f9b6c476063e1966a1c4")] | |||
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.CodeAnalyzer.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010015c01ae1f50e8cc09ba9eac9147cf8fd9fce2cfe9f8dce4f7301c4132ca9fb50ce8cbf1df4dc18dd4d210e4345c744ecb3365ed327efdbc52603faa5e21daa11234c8c4a73e51f03bf192544581ebe107adee3a34928e39d04e524a9ce729d5090bfd7dad9d10c722c0def9ccc08ff0a03790e48bcd1f9b6c476063e1966a1c4")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InternalsVisibleTo [](start = 11, length = 18)
Can remove. This was an artifact of an earlier attempt to allow the test access to the attribute via direct reference, instead of a recompilation of the source file.
private const string Category = "Access"; | ||
internal const string DiagnosticId = "MSML_NoBestFriendInternal"; | ||
|
||
private const string Title = "Cross-assembly internal access requires."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha ha ha. Have I ever done this before? 😄 Thanks @eerhardt
"reference, and the declaring assembly wants these accesses to be on something " + | ||
"with the attribute " + AttributeName + "."; | ||
private const string Description = | ||
"The ML.NET ."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this like `THE Ohio State University"? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly like that, yes.
|
||
// Get the symbols of the key types we are analyzing. If we can't find any of them there is | ||
// no point in going further. | ||
var attrType = comp.GetTypeByMetadataName(AttributeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to rename attrType
to bestFriendAttributeType
? Similar for assemblyAttrType
=> wantsToBeBestFriendsAttributeType
?
It gets a little confusing below when you see attrType
, and you kind of think "which attribute are we talking about here?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I was going for with the attr
vs. assemblyAttr
to track the two, but certainly it doesn't hurt to be more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7b2be87
to
8c6210c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #1519 .